Skip to content

Add keepalive to responseInputStream timeout scheduler executor to ma…#6756

Open
RanVaknin wants to merge 5 commits intomasterfrom
rvaknin/fix-thread-leak-in-toInputStream
Open

Add keepalive to responseInputStream timeout scheduler executor to ma…#6756
RanVaknin wants to merge 5 commits intomasterfrom
rvaknin/fix-thread-leak-in-toInputStream

Conversation

@RanVaknin
Copy link
Contributor

@RanVaknin RanVaknin commented Feb 28, 2026

Addresses: #6567

Background and context

ResponseInputStream uses a static ScheduledExecutorService to abort streams that aren't read within 60 seconds (to prevent connection leaks). The executor's thread never terminates because core threads are kept alive indefinitely by default. This causes the response-input-stream-timeout-scheduler thread to persist for the lifetime of the JVM, even when no streams exist.

To fix this, we can set allowCoreThreadTimeOut(true) and a 60s keep-alive on the executor. The thread now dies after 60 seconds of idleness and respawns on demand (matches TransferManagerConfiguration.java).

Testing

Since there is a timeout and a keep alive mechanism it's hard to write a deterministic test to prove the fix works. Additionally, adding a test with a sleep method of >1min is not feasible.
The following test fails on Master, and passes after the code change introduced in the PR:

    @Test
    void schedulerThread_shouldTerminateAfterIdleTimeout() throws Exception {
        ResponseInputStream<Object> responseInputStream = responseInputStream(Duration.ofSeconds(1));
        responseInputStream.read();
        responseInputStream.close();

        Thread.sleep(65000);

        Set<String> schedulerThreads = new HashSet<>();

        for (Thread thread : Thread.getAllStackTraces().keySet()) {
            if (thread.getName().contains("response-input-stream-timeout-scheduler")) {
                schedulerThreads.add(thread.getName());
            }
        }
        assertThat(schedulerThreads).isEmpty();
    }

@RanVaknin RanVaknin requested a review from a team as a code owner February 28, 2026 00:02
@RanVaknin RanVaknin added the no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required label Mar 2, 2026
zoewangg
zoewangg requested changes Mar 2, 2026
View reviewed changes
core/sdk-core/src/main/java/software/amazon/awssdk/core/ResponseInputStream.java
static final ScheduledExecutorService INSTANCE;

static {
ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1, r -> {
Copy link
Contributor

@zoewangg zoewangg Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, we can probably just do INSTANCE = new ScheduledThreadPoolExecutor(...?

Copy link
Contributor Author

@RanVaknin RanVaknin Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we can do this.
setKeepAliveTime and allowCoreThreadTimeOut are methods on ScheduledThreadPoolExecutor (concrete class), not on ScheduledExecutorService (interface).

Since INSTANCE is the interface, we need a local variable of the concrete type to call those methods
before assigning to INSTANCE.

core/sdk-core/src/main/java/software/amazon/awssdk/core/ResponseInputStream.java Outdated
t.setDaemon(true);
return t;
});
executor.setKeepAliveTime(DEFAULT_TIMEOUT.getSeconds(), TimeUnit.SECONDS);
Copy link
Contributor

@zoewangg zoewangg Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: DEFAULT_TIMEOUT is the stream read timeout (how long before aborting an unread stream). The keep-alive is how long an idle thread survives. These are conceptually different values that happen to be 60s today. If someone changes DEFAULT_TIMEOUT in the future, the thread keep-alive silently changes too. Consider a separate constant

Copy link
Contributor Author

@RanVaknin RanVaknin Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call out, I'll add a separate constant.

Since currently it uses DEFAULT_TIMEOUT What do you think about the 60s timeout of the scheduler thread? Do we want to make it shorter? ie; 15 seconds?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
2.6% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zoewangg zoewangg zoewangg approved these changes

Assignees

No one assigned

Labels

no-api-surface-area-change Indicate there is no API surface area change and thus API surface area review is not required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@RanVaknin @zoewangg